Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not send empty test suite spans #5405

Merged
merged 1 commit into from
Jun 22, 2023
Merged

Do not send empty test suite spans #5405

merged 1 commit into from
Jun 22, 2023

Conversation

nikita-tkachenko-datadog
Copy link
Contributor

@nikita-tkachenko-datadog nikita-tkachenko-datadog commented Jun 15, 2023 •
edited
Loading

What Does This Do

Adds a check to avoid reporting empty test suite spans.
A test suite span is considered empty if it has no children (i.e. suite does not include test cases) and it was not explicitly reported by the test framework as skipped or failed.

Motivation

In certain edge cases a test framework might report a test suite as started, and then all of suite's children might be skipped by Intelligent Test Runner (this happens with parameterized tests in JUnit 5 - the list of test cases is not known in advance, so when suite is started there is no way to know if it will be skipped entirely or not).
These empty suites are confusing for the product's users and must not be reported.

@nikita-tkachenko-datadog nikita-tkachenko-datadog added type: bug comp: ci visibility Continuous Integration Visibility labels Jun 15, 2023
@nikita-tkachenko-datadog nikita-tkachenko-datadog changed the title Do not send empty test suite spans WIP: Do not send empty test suite spans Jun 15, 2023
@nikita-tkachenko-datadog nikita-tkachenko-datadog force-pushed the nikita-tkachenko/do-not-send-spans-for-empty-suites branch 2 times, most recently from 46afe94 to c1f09e8 Compare June 15, 2023 17:09
@pr-commenter
Copy link

pr-commenter bot commented Jun 15, 2023 •
edited
Loading

Benchmarks

Parameters

Baseline Candidate
commit 1.17.0-SNAPSHOT~2e03d43a32 1.17.0-SNAPSHOT~37e559a7cc
config baseline candidate
See matching parameters
Baseline Candidate
module Agent Agent
parent None None

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 22 cases.

@nikita-tkachenko-datadog nikita-tkachenko-datadog changed the title WIP: Do not send empty test suite spans Do not send empty test suite spans Jun 19, 2023
@nikita-tkachenko-datadog nikita-tkachenko-datadog force-pushed the nikita-tkachenko/move-classes-to-civisibility-from-api branch from 27b4c27 to 80e26c4 Compare June 19, 2023 16:58
Base automatically changed from nikita-tkachenko/move-classes-to-civisibility-from-api to nikita-tkachenko/per-test-code-coverage-auto-config-maven June 20, 2023 10:28
Base automatically changed from nikita-tkachenko/per-test-code-coverage-auto-config-maven to master June 20, 2023 12:41
@nikita-tkachenko-datadog nikita-tkachenko-datadog force-pushed the nikita-tkachenko/do-not-send-spans-for-empty-suites branch from c1f09e8 to 37e559a Compare June 20, 2023 12:43
@nikita-tkachenko-datadog nikita-tkachenko-datadog marked this pull request as ready for review June 20, 2023 14:27
@nikita-tkachenko-datadog nikita-tkachenko-datadog requested a review from a team as a code owner June 20, 2023 14:27
PerfectSlayer
PerfectSlayer approved these changes Jun 22, 2023
View reviewed changes
Copy link
Contributor

@PerfectSlayer PerfectSlayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed context.

Quick question: any reason the context status is a bunch of cached String in an interface rather than an enum?
It always feel bad to me to have null check everywhere in its codebase. I would have even define an "unknown" status if needed to prevent it.

Otherwise, it's good to me.

@nikita-tkachenko-datadog
Copy link
Contributor Author

Quick question: any reason the context status is a bunch of cached String in an interface rather than an enum? It always feel bad to me to have null check everywhere in its codebase. I would have even define an "unknown" status if needed to prevent it.

I guess I just got used to them being Strings and stopped noticing the inconvenience at some point. An enum is definitely a better choice. Although switching to an enum would require some effort (there are 100+ usages of the String constants in the codebase). I am surely going to do this, but outside of this PR's scope

@nikita-tkachenko-datadog nikita-tkachenko-datadog merged commit 9799c17 into master Jun 22, 2023
@nikita-tkachenko-datadog nikita-tkachenko-datadog deleted the nikita-tkachenko/do-not-send-spans-for-empty-suites branch June 22, 2023 15:10
@github-actions github-actions bot added this to the 1.17.0 milestone Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PerfectSlayer PerfectSlayer PerfectSlayer approved these changes

Assignees
No one assigned
Labels
comp: ci visibility Continuous Integration Visibility type: bug
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.
2 participants
@nikita-tkachenko-datadog @PerfectSlayer